Fix test broker leaks, state races, and signal-masked command failures#541
Fix test broker leaks, state races, and signal-masked command failures#541apedroheringer wants to merge 1 commit into
Conversation
- tests/helpers.mjs: track temp workspaces and add a global after() teardown that shuts down any broker a test left running (graceful shutdown, then terminateProcessTree escalation) and removes its session dir; the suite now fails loudly if a broker survives teardown. - tests/runtime.test.mjs: the lazy-broker status test now runs the SessionEnd hook like its neighbors instead of leaking its broker. - lib/locking.mjs (new): minimal mkdir-based advisory lock with stale reclaim, sync and async variants. - lib/state.mjs: serialize saveState/updateState per workspace and write state.json via temp file + fsync + atomic rename, so concurrent read-modify-write cycles no longer lose updates or tear the file. - lib/broker-lifecycle.mjs: serialize ensureBrokerSession's check-then-create sequence per workspace so concurrent callers cannot spawn duplicate brokers. - lib/process.mjs: stop coalescing a null spawnSync status to 0 when the process died from a signal; runCommandChecked now reports it as a failure. - New regression tests: signal-terminated commands, concurrent upsertJob from two processes, concurrent ensureBrokerSession sharing one broker.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 878c86314f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (pidAlive(pid)) { | ||
| throw new Error(`Leaked broker process ${pid} survived suite teardown.`); |
There was a problem hiding this comment.
In environments where the detached broker has exited but remains as a zombie under PID 1, process.kill(pid, 0) still succeeds, so this new teardown path reports a leak that cannot be terminated. I hit this with npm test: all tests passed, then this hook failed with Leaked broker process ..., and ps showed that PID as [MainThread] <defunct>. That makes the suite fail nondeterministically depending on how quickly the container init reaps orphaned detached brokers; the leak check needs to distinguish defunct processes from live brokers before throwing.
Useful? React with 👍 / 👎.
1 participant
Problem
Running
npm testleaves dozens of orphaned processes behind (brokerapp-server-broker.mjs serveinstances plus fakecodex app-serverchildren spawned from/tmp/codex-plugin-test-*workspaces). On one machine a single suite run left ~30 brokers + ~29 fake app-servers running after the temp dirs had already been cleaned up. Root causes, verified againstmain(db52e28):SessionEndhook (e.g. the lazy-brokerstatustest attests/runtime.test.mjs:2209, while its neighbours at:2153/:2198do clean up). Any test failure or earlyreturnalso skips cleanup.state.jsonwrites are a read-modify-write with no lock and no atomic rename (lib/state.mjs—saveState/updateStateuse a barefs.writeFileSync). Two concurrent commands can lose updates or tear the file.ensureBrokerSessionis check-then-create with no serialization (lib/broker-lifecycle.mjs). Two concurrent callers can both miss the existing broker and spawn duplicates, orphaning one.lib/process.mjscoalescesspawnSync'sstatus: nullto0, sorunCommandCheckedtreats a SIGTERM/SIGKILL death as exit 0.Changes
tests/helpers.mjs:makeTempDirnow tracks created workspaces, and a globalafter()hook shuts down any broker a test left running (gracefulbroker/shutdown, thenterminateProcessTreeescalation), removes its session dir, and fails the suite loudly if a broker survives teardown — so leaks cannot reappear silently.tests/runtime.test.mjs: the lazy-brokerstatustest now runsSessionEndlike its neighbours.lib/locking.mjs(new): minimal mkdir-based advisory lock with stale reclaim; sync and async variants; no new dependencies.lib/state.mjs:saveState/updateStateare serialized per workspace and write via temp file +fsync+ atomicrename.lib/broker-lifecycle.mjs: the check-then-create sequence inensureBrokerSessionis serialized per workspace.lib/process.mjs:status: nullwith a signal is normalized to a non-zero status;runCommandCheckednow reportssignal=SIGTERMfailures.Testing
npm test: 95/95 passing (91 existing + 4 new regression tests: signal-terminated command reported as failure ×2, concurrentupsertJobfrom two processes loses no updates, concurrentensureBrokerSessioncalls share a single broker).npm run check-versionpasses.Related work
lib/process.mjsfix here addresses one path where a signal death is reported as success.🤖 Generated with Claude Code
https://claude.ai/code/session_01K9WP1dAojgJe3xXJMV9DM1